Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add libcudf wrappers around current_device_resource functions. #16679

Merged
merged 25 commits into from
Sep 10, 2024
Merged

Add libcudf wrappers around current_device_resource functions. #16679

merged 25 commits into from
Sep 10, 2024

Conversation

harrism
Copy link
Member

@harrism harrism commented Aug 28, 2024 •
edited
Loading

Description

Merge after rapidsai/rmm#1661

Creates and uses CUDF internal wrappers around RMM current_device_resource functions.

I've marked this PR as breaking because it breaks the ABI, however the API is compatible.

For reviewers, the most substantial additions are in the new file <cudf/utilities/memory_resource.hpp>, and in the DEVELOPER_GUIDE.md and *.rst docs. The rest are all replacements of an include and all calls to rmm::get_current_device_resource() with cudf::get_current_device_resource_ref().

Closes #16676

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. Java Affects Java cuDF API. labels Aug 28, 2024
@harrism harrism changed the title Fea/cudf_current_resource_wrappers Add libcudf wrappers around current_device_resource functions. Aug 28, 2024
@harrism harrism added feature request New feature or request breaking Breaking change labels Aug 28, 2024
@harrism harrism mentioned this pull request Aug 28, 2024
Open
@harrism harrism marked this pull request as ready for review August 28, 2024 21:38
@harrism harrism requested review from a team as code owners August 28, 2024 21:38
@harrism
Copy link
Member Author

harrism commented Aug 28, 2024

@vyasr I could use your help understanding the docs build failure. Thanks!

@harrism harrism mentioned this pull request Sep 3, 2024
Merged
@vyasr
Copy link
Contributor

vyasr commented Sep 4, 2024

You'll need to add a new docs page for memory_resource.hpp to https://github.com/rapidsai/cudf/tree/branch-24.10/docs/cudf/source/libcudf_docs/api_docs, and then add it either to index.rst or to one of the subpages that it lists in its ToC (or their subpages, etc).

@harrism
Copy link
Member Author

harrism commented Sep 4, 2024

You'll need to add a new docs page for memory_resource.hpp to https://github.com/rapidsai/cudf/tree/branch-24.10/docs/cudf/source/libcudf_docs/api_docs, and then add it either to index.rst or to one of the subpages that it lists in its ToC (or their subpages, etc).

Thanks! Made an attempt.

@nvdbaranec
Copy link
Contributor

nvdbaranec commented Sep 5, 2024 •
edited
Loading

This may have been asked already, but why do we have to include <cudf/utilities/default_stream.hpp> as well as <cudf/utilities/memory_resource.hpp> ? Or were the few places where this was done because it wasn't explicitly included for the neighboring cudf::get_default_stream?

nvdbaranec
nvdbaranec reviewed Sep 5, 2024
View reviewed changes
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a handful of places where the default stream header is included by ultimately unused. Example:

cudf/cpp/include/cudf/reduction/detail/segmented_reduction_functions.hpp

Line 22 in f83b64d

#include <cudf/utilities/default_stream.hpp>

Edit: Actually I think this may be the only one.

@harrism
Copy link
Member Author

harrism commented Sep 5, 2024 •
edited
Loading

This may have been asked already, but why do we have to include <cudf/utilities/default_stream.hpp> as well as <cudf/utilities/memory_resource.hpp> ? Or were the few places where this was done because it wasn't explicitly included for the neighboring cudf::get_default_stream?

Yes, I fixed IWYU mistakes when I noticed them.

nvdbaranec
nvdbaranec approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@nvdbaranec nvdbaranec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just that one lingering unnecessary header - not a blocker.

davidwendt
davidwendt reviewed Sep 9, 2024
View reviewed changes
cpp/include/cudf/io/text/detail/tile_state.hpp Outdated
@@ -17,8 +17,7 @@
#pragma once

#include <cudf/utilities/export.hpp>

#include <rmm/resource_ref.hpp>
#include <cudf/utilities/memory_resource.hpp>
Copy link
Contributor

@davidwendt davidwendt Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this include can be removed.

davidwendt
davidwendt reviewed Sep 9, 2024
View reviewed changes
cpp/include/cudf/io/text/detail/trie.hpp Outdated
@@ -19,11 +19,11 @@
#include <cudf/detail/utilities/vector_factories.hpp>
#include <cudf/io/text/detail/multistate.hpp>
#include <cudf/utilities/export.hpp>
#include <cudf/utilities/memory_resource.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this include can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, as well as device_memory_resource.hpp, but need to add rmm/resource_ref.hpp.

@harrism
Copy link
Member Author

harrism commented Sep 10, 2024

/merge

@rapids-bot rapids-bot bot merged commit afd3a4b into rapidsai:branch-24.10 Sep 10, 2024
98 checks passed
@hyperbolic2346 hyperbolic2346 mentioned this pull request Sep 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@davidwendt davidwendt davidwendt approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

@ttnghia ttnghia ttnghia approved these changes

@isVoid isVoid Awaiting requested review from isVoid isVoid is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@vyasr vyasr Awaiting requested review from vyasr

Assignees
No one assigned
Labels
breaking Breaking change feature request New feature or request Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
RMM Project Board
Status: Done
libcudf
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Add libcudf cudf::get_current_device_resource() wrapper for rmm::get_current_device_resource()
6 participants
@harrism @vyasr @nvdbaranec @ttnghia @galipremsagar @davidwendt